Fix duration_since panic on unix when std is built with integer overflow checks by stepancheg · Pull Request #146556 · rust-lang/rust

stepancheg · 2025-09-14T20:48:55Z

Add a test for regression #146228, and turns out this test fails detects error when std is compiled with integer overflow checks.

Original regression was reverted in #146473.

First attempt to fix was in #146247; test and some code is copied from there, thanks @eval-exec

r? @RalfJung

stepancheg · 2025-09-14T21:39:13Z

Seems like the bug was there before constify of SystemTime, but did not manifest in debug builds. I'll try to fix.

stepancheg · 2025-09-14T21:55:05Z

If anyone is around, not do I compile std with debug assertions including integer overflow? I'm running

./x test library/std --no-doc

and such assertions are not triggered.

workingjubilee · 2025-09-14T21:56:50Z

put this in your bootstrap.toml

rust.debug-assertions-std = true

from

rust/bootstrap.example.toml

Lines 603 to 604 in 52618eb

    
           # Defaults to rust.debug-assertions value 
        
           #rust.debug-assertions-std = rust.debug-assertions (boolean)

stepancheg · 2025-09-14T22:02:56Z

~~@workingjubilee this seems to enable debug_assert!, but does not trigger integer overflow checks.~~

stepancheg · 2025-09-14T22:07:52Z

Found it.

rust.overflow-checks = true

workingjubilee · 2025-09-14T22:29:48Z

Ah, yes, down a few lines then.

RalfJung · 2025-09-15T07:06:52Z

r? libs
since this actually changes the implementation

library/std/tests/time.rs

stepancheg · 2025-09-17T16:11:20Z

Ping @tgross35.

joboet · 2025-09-18T15:05:01Z

library/std/src/sys/pal/unix/time.rs

            } else {
                (
-                    (self.tv_sec - other.tv_sec - 1) as u64,
+                    (self.tv_sec - 1).wrapping_sub(other.tv_sec) as u64,


I had to think for a few seconds before I realised why the - 1 couldn't panic (since other.tv_nsec is larger than self.tv_nsec, but other smaller than self, other.tv_sec must be smaller than self.tv_sec, meaning that there is a value smaller than self.tv_sec, making the subtraction work). Could you perhaps add a comment to explain it? Or use wrapping_sub for the decrement as well?

I added a few assertions, so that sequence of assertion will explain why it does not underflow.

Or use wrapping_sub for the decrement as well?

That would not be right, because if there is a bug here, instead of debug assertion, we may quietly get wrong result.

tgross35 · 2025-09-19T03:45:53Z

library/std/src/sys/pal/unix/time.rs

@@ -150,12 +150,12 @@ impl Timespec {
            // directly expresses the lower-cost behavior we want from it.


The comment above here needs to be updated, considering it specifically calls out self.tv_sec - 1 - other.tv_sec vs. self.tv_sec - other.tv_sec - 1. I don't think it's all relevant anymore, given https://rust.godbolt.org/z/1Ex913qr5

I removed the comment because:

you asked to update it

I don't understand it

this does not seem like performance critical code to do

tgross35 · 2025-09-19T03:48:27Z

Ping @tgross35.

Hey, this was open for only three days :) I don't think this is high priority?

stepancheg · 2025-09-19T17:49:03Z

@tgross35

I don't think this is high priority?

The bugfix is not high priority, but I wanted to add a couple PRs on top, and if each one takes weeks to review, I will lose context and/or switch to some other work (my primary work is far from opensource).

Sorry for the ping. Is there expected time to a comment from the reviewer for a PR in rust repo so next time I'll avoid pinging the maintainers too early?

RalfJung · 2025-09-19T18:59:43Z

The comment that the bot posts for new contributors says

They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

tgross35

Thanks for submitting this fix, two small requests then r=me.

The bugfix is not high priority, but I wanted to add a couple PRs on top, and if each one takes weeks to review, I will lose context and/or switch to some other work (my primary work is far from opensource).

Sorry for the ping. Is there expected time to a comment from the reviewer for a PR in rust repo so next time I'll avoid pinging the maintainers too early?

No worries, we just usually expect a couple of weeks. If you have a small change, it's fine to ask on Zulip if there's anyone willing to take a quick look at something.

I think stacking PRs is pretty common to keep future work unblocked.

View changes since this review

library/std/src/sys/pal/unix/time.rs

rustbot · 2025-09-24T19:34:34Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

stepancheg · 2025-09-24T19:46:42Z

Applied suggestions.

Added helper function sub_ge_to_unsigned with debug assertion and explanations how to subtract i64 to u64.

And slight offtopic here, that is the function I needed a few times: shouldn't it it added to core? Something like:

impl i64 {
  // if self >= other { Ok(self - other) } else { Err(other - self) }
  fn sub_to_unsigned(self, other: i64) -> Result<u64, u64> {
    if self >= other {
      Ok(self.wrapping_sub(other).cast_unsigned())
    } else {
      Err(other.wrapping_sub(self).cast_unsigned())
    }
  }

  fn strict_sub_to_unsigned(self: i64, other: i64) -> u64 {
    // panic if ...
  }
}

This function would be somewhat similar to SystemTime::duration_since.

tgross35 · 2025-09-24T20:04:54Z

Looks great, thanks!

stepancheg · 2025-09-24T20:08:14Z

... last minute typo in comment fix.

tgross35 · 2025-09-24T20:52:16Z

@bors r+

bors · 2025-09-24T20:52:19Z

📌 Commit 92859e9 has been approved by tgross35

It is now in the queue for this repository.

tgross35 · 2025-09-24T20:56:15Z

@bors rollup

Rollup of 7 pull requests Successful merges: - #146556 (Fix duration_since panic on unix when std is built with integer overflow checks) - #146679 (Clarify Display for error should not include source) - #146753 (Improve the pretty print of UnstableFeature clause) - #146894 (Improve derive suggestion of const param) - #146950 (core: simplify `CStr::default()`) - #146958 (Fix infinite recursion in Path::eq with String) - #146971 (fix ICE in writeback due to bound regions) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #146556 - stepancheg:repro-146228, r=tgross35 Fix duration_since panic on unix when std is built with integer overflow checks Add a test for regression #146228, and turns out this test fails detects error when std is compiled with integer overflow checks. Original regression was reverted in #146473. First attempt to fix was in #146247; test and some code is copied from there, thanks `@eval-exec` r? `@RalfJung`

Rollup of 7 pull requests Successful merges: - rust-lang/rust#146556 (Fix duration_since panic on unix when std is built with integer overflow checks) - rust-lang/rust#146679 (Clarify Display for error should not include source) - rust-lang/rust#146753 (Improve the pretty print of UnstableFeature clause) - rust-lang/rust#146894 (Improve derive suggestion of const param) - rust-lang/rust#146950 (core: simplify `CStr::default()`) - rust-lang/rust#146958 (Fix infinite recursion in Path::eq with String) - rust-lang/rust#146971 (fix ICE in writeback due to bound regions) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#146556 (Fix duration_since panic on unix when std is built with integer overflow checks) - rust-lang#146679 (Clarify Display for error should not include source) - rust-lang#146753 (Improve the pretty print of UnstableFeature clause) - rust-lang#146894 (Improve derive suggestion of const param) - rust-lang#146950 (core: simplify `CStr::default()`) - rust-lang#146958 (Fix infinite recursion in Path::eq with String) - rust-lang#146971 (fix ICE in writeback due to bound regions) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned RalfJung Sep 14, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 14, 2025

stepancheg force-pushed the repro-146228 branch from 87b71c0 to f335c20 Compare September 14, 2025 20:52

stepancheg changed the title ~~Repro duration_since regression from issue 146228~~ Test for (reverted now) duration_since regression Sep 14, 2025

This comment has been minimized.

Sign in to view

stepancheg force-pushed the repro-146228 branch from f335c20 to 9cc8046 Compare September 14, 2025 22:22

rustbot added O-hermit Operating System: Hermit O-unix Operating system: Unix-like labels Sep 14, 2025

stepancheg changed the title ~~Test for (reverted now) duration_since regression~~ Fix duration_since panic on unix when std is built with integer overflow checks Sep 14, 2025

rustbot assigned tgross35 and unassigned RalfJung Sep 15, 2025

RalfJung reviewed Sep 15, 2025

View reviewed changes

library/std/tests/time.rs Outdated Show resolved Hide resolved

stepancheg force-pushed the repro-146228 branch from 9cc8046 to 162ee46 Compare September 15, 2025 10:52

This comment has been minimized.

Sign in to view

joboet reviewed Sep 18, 2025

View reviewed changes

tgross35 reviewed Sep 19, 2025

View reviewed changes

stepancheg force-pushed the repro-146228 branch from 162ee46 to 8d641ba Compare September 19, 2025 17:41

This comment has been minimized.

Sign in to view

tgross35 reviewed Sep 24, 2025

View reviewed changes

library/std/src/sys/pal/unix/time.rs Show resolved Hide resolved

library/std/src/sys/pal/unix/time.rs Outdated Show resolved Hide resolved

stepancheg force-pushed the repro-146228 branch from 8d641ba to dfc3e3e Compare September 24, 2025 19:34

stepancheg requested a review from tgross35 September 24, 2025 19:48

RalfJung mentioned this pull request Sep 24, 2025

SystemTime::duration_since nightly regression #146228

Closed

tgross35 approved these changes Sep 24, 2025

View reviewed changes

Repro duration_since regression from issue 146228

92859e9

stepancheg force-pushed the repro-146228 branch from dfc3e3e to 92859e9 Compare September 24, 2025 20:07

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2025

matthiaskrgr mentioned this pull request Sep 24, 2025

Rollup of 7 pull requests #147003

Merged

bors merged commit 8073b6f into rust-lang:master Sep 25, 2025
10 checks passed

rustbot added this to the 1.92.0 milestone Sep 25, 2025

		@@ -150,12 +150,12 @@ impl Timespec {
		// directly expresses the lower-cost behavior we want from it.

Uh oh!

Conversation

stepancheg commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

stepancheg commented Sep 14, 2025

Uh oh!

stepancheg commented Sep 14, 2025

Uh oh!

workingjubilee commented Sep 14, 2025

Uh oh!

stepancheg commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stepancheg commented Sep 14, 2025

Uh oh!

workingjubilee commented Sep 14, 2025

Uh oh!

RalfJung commented Sep 15, 2025

Uh oh!

Uh oh!

This comment has been minimized.

stepancheg commented Sep 17, 2025

Uh oh!

joboet Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

stepancheg Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stepancheg Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Sep 19, 2025

Uh oh!

This comment has been minimized.

stepancheg commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Sep 19, 2025

Uh oh!

tgross35 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rustbot commented Sep 24, 2025

Uh oh!

stepancheg commented Sep 24, 2025

Uh oh!

tgross35 commented Sep 24, 2025

Uh oh!

stepancheg commented Sep 24, 2025

Uh oh!

tgross35 commented Sep 24, 2025

Uh oh!

bors commented Sep 24, 2025

Uh oh!

tgross35 commented Sep 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

stepancheg commented Sep 14, 2025 •

edited

Loading

stepancheg commented Sep 14, 2025 •

edited

Loading

tgross35 Sep 19, 2025 •

edited

Loading

stepancheg commented Sep 19, 2025 •

edited

Loading

tgross35 left a comment •

edited by rustbot

Loading